-
-
Notifications
You must be signed in to change notification settings - Fork 447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AdminSiteOTPRequiredMixin modified #370
base: master
Are you sure you want to change the base?
Conversation
When admin user is staff user and is active, but doesnt have OTP setup, AdminSiteOTPRequiredMixin will redirect user to OTP setup page after login.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are required and imports need to be ordered (see failing Travis build)
@@ -44,21 +43,30 @@ def test_default_admin(self): | |||
|
|||
@override_settings(ROOT_URLCONF='tests.urls_otp_admin') | |||
class OTPAdminSiteTest(UserMixin, TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I test this class alone, all tests pass.
When I run entire test suit, 2 tests in this class fail
- test_otp_admin_without_otp
- test_otp_admin_without_otp_named_url
When running entire test suite, self.client.get('/otp_admin/', follow=True)
returns login page url /account/login/?next=%2Fotp_admin%2F,
when its supposed to return a OTP setup url /account/two_factor/setup/
.
I was unable to find why this happens.
Any help is appreciated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I just test this class: AdminSiteOTPRequiredMixin().login() func executes.
When I run entire test suite - AdminSiteOTPRequiredMixin().login() func never executes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not had a chance to look at this yet and I'm not overly familiar with the admin code.
I would imagine that our tests aren't quite as isolated from eachother as we'd actually want. Have you tried running just the test_admin
module and then the class those two tests belong to? That might help narrow down where the problem is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept AdminSiteTest
and OTPAdminSiteTest
in test_admin.py and tried to test make test TARGET=tests.test_admin
They work fine as long as there is no get request made in AdminSiteTest
class.
If I make that get request in AdminSiteTest.test_default_admin
I get error in OTPAdminSiteTest
class.
class AdminSiteTest(UserMixin, TestCase):
def setUp(self):
super().setUp()
self.user = self.create_superuser()
self.login_user()
def test_default_admin(self):
with self.settings(ROOT_URLCONF='tests.urls_admin'):
# if I comment the line below, test_admin.py tests successfully.
response = self.client.get('/admin/')
class OTPAdminSiteTest(UserMixin, TestCase):
def setUp(self):
super().setUp()
self.user = self.create_superuser()
self.login_user()
def test_otp_admin_without_otp(self):
with self.settings(ROOT_URLCONF='tests.urls_otp_admin'):
response2 = self.client.get('/otp_admin/', follow=True)
redirect_to = reverse('two_factor:setup')
self.assertRedirects(response2, redirect_to)
Error:
Response redirected to '/admin/', expected '/account/two_factor/setup/'
So in conclusion, error in OTPAdminSiteTest
class depends on which test was run previously. If I were to substitute AdminSiteTest
with AdminPatchTest
in the above code, my error will change to following:
Response redirected to '/account/login/?next=/otp_admin/', expected '/account/two_factor/setup/
redirect_to = request.POST.get(REDIRECT_FIELD_NAME, request.GET.get(REDIRECT_FIELD_NAME)) | ||
|
||
# if user (is_active and is_staff) | ||
if request.method == "GET" and AdminSite().has_permission(request): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not self.has_permission(request)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.has_permission()
func checks if user (is_staff, is_active and is_verified) .
AdminSite().has_permission()
checks if user (is_staff, is_active ). Then based on if user is not verified (OTP not setup) we want to redirect to OTP setup page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super()
should be used instead then. This is a mixin and we don't know what other checks users might have/want so there might be other classes involved, not just AdminSite
.
Can anyone help please? |
@aseem-hegshetye @moggers87 any updates on this? Can I help? I'd like this feature. |
When admin user is staff user and is active, but doesnt have OTP setup, AdminSiteOTPRequiredMixin will redirect user to OTP setup page after login.